-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove dead code from DisplayServerWindows::window_set_size
#86029
Conversation
DisplayServerWindows::window_set_size
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. window_resize()
is just a stub for Windows while window_resize
is typically called as a consequence of MoveWindow()
(it is only not called when the size is not being changed).
This PR will need a rebase, because context_vulkan
was renamed. But it will be a trivial rebase
7b26a20
to
f7e8d63
Compare
#if defined(GLES3_ENABLED) | ||
if (gl_manager_native) { | ||
gl_manager_native->window_resize(p_window, w, h); | ||
} | ||
if (gl_manager_angle) { | ||
gl_manager_angle->window_resize(p_window, w, h); | ||
} | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GL resize part (even if it's not really doing anything right now) probably should be moved to WM_WINDOWPOSCHANGED
instead of removing it, currently it's only calling RD window_set_sized
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, to be fair I didn't retest this, but when I initially pushed this PR it worked just fine without. And it still should, the window can be resized externally without ever calling window_set_size
so the code to handle resizing needs to be already present somewhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the Windows GL manager implementation these functions do nothing, so it should work fine without it. But I think it should call it from WM_WINDOWPOSCHANGED
anyway, just in case GL manager ever change, and for consistency with other platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see what you mean. Will add it
a55dcb1
to
24a61a0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested, works fine here with no validation errors!
Thanks! |
The removed code will in fact just recreate the swapchain with the same parameters it is already created with. While
context_vulkan->window_resize(p_window, w, h);
is called with the updated parameters,godot/drivers/vulkan/vulkan_context.cpp
Line 1981 in a9f444b
Also
MoveWindow
is a synchronous call, meaning this line here will be executed before the function returnsgodot/platform/windows/display_server_windows.cpp
Line 3773 in a9f444b
So currently calling
window_set_size
will recreate the swapchain twice.